-
-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prepare for working with newer versions of OpenSwoole. #128
base: 4.10.x
Are you sure you want to change the base?
Conversation
16f59a1
to
c5ec024
Compare
I don't think changing autoloading rules affects lock file. Those fields are not used for hash afaik |
Signed-off-by: Slavey Karadzhov <skaradzhov@perforce.com>
c5ec024
to
faa15c1
Compare
compat/openswoole.php
Outdated
|
||
// @see https://github.com/mezzio/mezzio-swoole/issues/110#issuecomment-1500174967 | ||
// Override the swoole_set_process_name function | ||
if (version_compare(phpversion('openswoole'), '22.0.0', '>=')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should check if openswoole extension is installed first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, if openswoole users added this workaround to their code this will cause errors. So this should also be wrapped in function_exists.
Verification is needed that vendor autoload files are included after root composer.json autoloading rules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, if openswoole users added this workaround to their code this will cause errors.
If there is a migration document I can document this. Ideally this package should take care of such differences and not the end user. I guess end users will be more than happy to have mezzio-swoole working out of the box without the need for them to provide custom code.
@slaff I am not an author here, btw. |
@Xerkus thank you anyway. If you can, please, tell me whom shall I credit and I will put their names. |
Signed-off-by: Slavey Karadzhov <skaradzhov@perforce.com>
f2b2617
to
01a247d
Compare
Signed-off-by: Slavey Karadzhov <skaradzhov@perforce.com>
I am confused as to why the project is introducing a compatibility layer for the current version of OpenSwoole, and it is not refactoring to continue 1st party with OpenSwoole, and add a compatibility layer in case Swoole is used, if the effort is worth it. The Mezzio Swoole documentation instructs users to use OpenSwoole, not Swoole. If OpenSwoole 24.x, 26.x will diverge more from Swoole (which will happen), wouldn't users be aware that the package is focused on Swoole, opposite to what they are instructed by docs for OpenSwoole? I'd be happy to see the package in newer versions requiring openswoole > 22. |
Me too. But it is up to the project leaders and contributors to decide which direction to go. |
@slaff Anything else to add to this PR? |
Make Mezzio Compatible with OpenSwoole v 22.0 and newer
This PR adds initial compatibility with OpenSwoole v22.0 and newer.
The changes in the PR are inspired by @Xerkus work and all credit goes to him.
This PR is related to #110.
TODO